Merge "extra" lint target into "normal" lint target #153

Akhilesh-max · 2025-01-13T11:16:43Z

Fixes -- #91

This PR merges the "extra" lint target into the "normal" lint target in the Makefile.

Akhilesh-max · 2025-01-13T14:52:10Z

Hi @thomas-fossati, I have merged the "extra" lint into the "normal" lint target. Most of the errors have been fixed, and some have been suppressed wherein reasonable to do so. I have also updated the workflow file. PTAL when you get a chance.

Thanks!!

cots/test_vars.go

Akhilesh-max · 2025-01-14T11:08:03Z

I’ve made the suggested changes. Please review them when get a chance. @setrofim, @yogeshbdeshpande, @thomas-fossati

Thanks!

setrofim · 2025-01-14T11:21:20Z

Please could you also squash the fixup commits.

yogeshbdeshpande · 2025-01-14T11:37:10Z

Thanks @Akhilesh-max : Reviewing it now but in the mean time, please follow the squash advise from Sergei

yogeshbdeshpande · 2025-01-14T11:44:59Z

comid/example_psa_refval_test.go

-	for i, m := range m.Values {
-		if err := extractSwMeasurement(m); err != nil {
+	for i := range m.Values {
+		meas := &m.Values[i]


traditionally golang always follows conciseness!

Wondering what lint warning is thrown for this change, I mean is line 73 is this change really needed ..?

Could you please clarify.

The old L72 makes a copy of m at each iteration.
The new L72 takes a reference instead.

Then should not it be better to set extractSwMeasurement(&m)
Or if copy needs to be avoided, then

extractSwMeasurement(&meas.Values[i])

Then should not it be better to set extractSwMeasurement(&m)

hmm no, that changes nothing wrt the copy. It's already happened

Or if copy needs to be avoided, then

extractSwMeasurement(&meas.Values[i])

I guess you mean extractSwMeasurement(&m.Values[i])?
If so, it'd be equivalent to the proposed change, except it's a bit less readable.

Then should not it be better to set extractSwMeasurement(&m)

hmm no, that changes nothing wrt the copy. It's already happened

True that , however now we copy a pointer instead of a value, how different then it is to the original change ?

Or if copy needs to be avoided, then
extractSwMeasurement(&meas.Values[i])

I guess you mean extractSwMeasurement(&m.Values[i])? If so, it'd be equivalent to the proposed change, except it's a bit less readable.

ok, but this avoids one extra variable addition, so much neater, in my view!

Then should not it be better to set extractSwMeasurement(&m)

hmm no, that changes nothing wrt the copy. It's already happened

True that , however now we copy a pointer instead of a value, how different then it is to the original change ?

In yours, the copy has already happened in the for statement, you are not avoiding it by taking the reference to m: it's too late.
In the proposed change, the measurement object is not copied.

Or if copy needs to be avoided, then
extractSwMeasurement(&meas.Values[i])

I guess you mean extractSwMeasurement(&m.Values[i])? If so, it'd be equivalent to the proposed change, except it's a bit less readable.

ok, but this avoids one extra variable addition, so much neater, in my view!

Readability is, by definition, in the eye of the reader :-)
Personally, I do not like cramming too much information into a single line, therefore I prefer the proposed change.
Anyway, this is not a hill I'd die on :-) I'll let @Akhilesh-max decide.

Honestly, i did not like the suggested change from lint-extra, it kind of defeats the purpose of how effectively one uses golang for loop, to have access to its index and its variables in a single sentence.

yogeshbdeshpande

Thanks for the changes, I have reviewed it: Most of the changes looks good to me, except one question I put forth in the Review

thomas-fossati · 2025-01-14T12:22:22Z

comid/comid.go

@@ -242,7 +242,7 @@ func (o *Comid) AddDevIdentityKey(val KeyTriple) *Comid {
 	return o
 }

-func (o Comid) Valid() error {
+func (o *Comid) Valid() error {


Valid does not modify the Comid object.

Therefore, pass-by-reference conveys the wrong semantics.

I guess this is gocritic's hugeParam check? If so, we might as well silence it.
(Maybe even remove it altogether from the linter config.)

Will that not result in unnecessary copies of a large object?

I suppose, we shouldn't worry about perf until this can be shown to be a problem, but univerasally disabling the flag makes me feel a bit uneasy. (Makes me wish Go had mutable and immutable references a la Rust...)

Will that not result in unnecessary copies of a large object?

Yes, that is likely the thing we are warned about here.

I suppose, we shouldn't worry about perf until this can be shown to be a problem, but univerasally disabling the flag makes me feel a bit uneasy.

I'm OK with a local lint annotation.

Tangentially, I think we may want to also take a critical look at the checks we are enabling and whether they are the right set.

(Makes me wish Go had mutable and immutable references a la Rust...)

Grust FTW! :-D

Tangentially, I think we may want to also take a critical look at the checks we are enabling and whether they are the right set.

Agreed, though I do think hugeParam check is a reasonable thing to have enabled (I do remember it being useful thing to me in the past when unintentionally copying a larger structure than CoRIM).

Grust FTW! :-D

Lol :D. I'd love to see a more cargo-like build/dependency management system for Go as well...

@setrofim @thomas-fossati, via the conversation above, I am able to deduce two things --

i) We will use local lint annotations to silence some of the errors where it is rational, without disabling the hugeParam check. Shall I silence all the hugeParam linting errors on value receivers in the context of this comment?

ii) We need to review the enabled linter checks. (Created an issue for this - #155 )

i) Yes, please make that change. (Personally, I think what you have now is fine; however as Thomas wants to preserve the semantics of pointer receivers implying modification in the method, I'm onboard with suppressing the warning for now -- we don't care too much about perf at the moment, and the Comid structure is not massive -- I estimate ~90 bytes -- so a few additional copies is not a big deal).

ii) Thank you!

OK -- We have 26 hugeParam linting errors on value receivers. Below are the structs, their sizes, and the number of occurrences:
• EatCWTClaim: 176 bytes, 2 occurrences
• conciseTaStore: 120 bytes, 3 occurrences
• AbbreviatedSwidTag: 180 bytes, 4 occurrences
• unsignedCorim: 88 bytes, 4 occurrences
• Measurement: 128 bytes, 2 occurrences
• Mval: 112 bytes, 3 occurrences
• Flagsmap: 88 bytes, 4 occurrences
• Comid: 112 bytes, 4 occurrences

Is it OK to suppress hugeParam for all warnings?

Yeah, that all seems fine. Thanks for checking.

thomas-fossati · 2025-01-14T12:23:07Z

comid/comid.go

 }

 // FromJSON deserializes a JSON-encoded CoMID into the target Comid
 func (o *Comid) FromJSON(data []byte) error {
 	return encoding.PopulateStructFromJSON(data, o)
 }

-func (o Comid) ToJSONPretty(indent string) ([]byte, error) {
+func (o *Comid) ToJSONPretty(indent string) ([]byte, error) {


ditto (see https://github.com/veraison/corim/pull/153/files#r1914729206)

There are other instances of this.

Sure, would suppress the hugeParam check herein.
However, please suggest to the query in this comment.

thomas-fossati · 2025-01-14T12:35:56Z

comid/example_test.go

@@ -109,8 +109,8 @@ func Example_encode() {
 	}

 	// Output:
-	//a50065656e2d474201a10078206d792d6e733a61636d652d726f616472756e6e65722d737570706c656d656e740282a3006941434d45204c74642e01d8207468747470733a2f2f61636d652e6578616d706c6502820100a20069454d4341204c74642e0281020382a200781a6d792d6e733a61636d652d726f616472756e6e65722d626173650100a20078196d792d6e733a61636d652d726f616472756e6e65722d6f6c64010104a4008182a300a500d86f445502c000016941434d45204c74642e026a526f616452756e6e65720300040101d902264702deadbeefdead02d8255031fb5abf023e4992aa4e95f9c1503bfa81a200d8255031fb5abf023e4992aa4e95f9c1503bfa01aa01d90228020282820644abcdef00820644ffffffff03a201f403f504d9023044010203040544ffffffff064802005e1000000001075020010db8000000000000000000000068086c43303258373056484a484435094702deadbeefdead0a5031fb5abf023e4992aa4e95f9c1503bfa018182a300a500d8255031fb5abf023e4992aa4e95f9c1503bfa016941434d45204c74642e026a526f616452756e6e65720300040101d902264702deadbeefdead02d8255031fb5abf023e4992aa4e95f9c1503bfa81a200d8255031fb5abf023e4992aa4e95f9c1503bfa01aa01d90229020282820644abcdef00820644ffffffff03a300f401f403f504d9023044010203040544ffffffff064802005e1000000001075020010db8000000000000000000000068086c43303258373056484a484435094702deadbeefdead0a5031fb5abf023e4992aa4e95f9c1503bfa028182a101d902264702deadbeefdead81d9022a78b12d2d2d2d2d424547494e205055424c4943204b45592d2d2d2d2d0a4d466b77457759484b6f5a497a6a3043415159494b6f5a497a6a304441516344516741455731427671462b2f727938425761375a454d553178595948455138420a6c4c54344d46484f614f2b4943547449767245654570722f7366544150363648326843486462354845584b74524b6f6436514c634f4c504131513d3d0a2d2d2d2d2d454e44205055424c4943204b45592d2d2d2d2d038182a101d8255031fb5abf023e4992aa4e95f9c1503bfa81d9022a78b12d2d2d2d2d424547494e205055424c4943204b45592d2d2d2d2d0a4d466b77457759484b6f5a497a6a3043415159494b6f5a497a6a304441516344516741455731427671462b2f727938425761375a454d553178595948455138420a6c4c54344d46484f614f2b4943547449767245654570722f7366544150363648326843486462354845584b74524b6f6436514c634f4c504131513d3d0a2d2d2d2d2d454e44205055424c4943204b45592d2d2d2d2d


these seem gratuitous changes. If it's another gocritic lamentation we may want to suppress it.

Added a space between // and the comment text. This should be fine?

setrofim · 2025-01-14T13:21:00Z

Sorry to nit-pick, but could you also ammend the commit message to adhere to conventional commits practice and to be more informative. Something like

chore(make): merge extra-lint target into lint

Add extra-lint checks to lint target to ensure they are always run during CI,
and remove the need for an additional target that needs to be executed manually
(which is often neglected resulting in gradual accumulation of regressions on
main).

Akhilesh-max · 2025-01-15T10:34:24Z

@setrofim Squashed the fixup commits and amended the commit message. Thanks for the suggestion though :).

Add extra-lint checks to the lint target to ensure they are always run during CI, removing the need for an additional target that is often neglected. This helps prevent the accumulation of regressions in the main branch. Signed-off-by: Akhilesh Kr. Yadav <[email protected]>

setrofim

LGTM

Akhilesh-max · 2025-01-15T16:42:05Z

@yogeshbdeshpande @thomas-fossati PTAL.
Thanks!

yogeshbdeshpande

LGTM!

Akhilesh-max marked this pull request as ready for review January 13, 2025 11:16

Akhilesh-max force-pushed the remove-extra-lint branch from ce5d147 to 33723e5 Compare January 13, 2025 12:02

Akhilesh-max mentioned this pull request Jan 13, 2025

Merge "extra" lint target into "normal" lint target #152

Closed

setrofim reviewed Jan 14, 2025

View reviewed changes

cots/test_vars.go Outdated Show resolved Hide resolved

cots/test_vars.go Outdated Show resolved Hide resolved

yogeshbdeshpande reviewed Jan 14, 2025

View reviewed changes

yogeshbdeshpande requested changes Jan 14, 2025

View reviewed changes

Akhilesh-max force-pushed the remove-extra-lint branch from 5fe22a9 to 84f4b51 Compare January 14, 2025 11:49

thomas-fossati reviewed Jan 14, 2025

View reviewed changes

Akhilesh-max force-pushed the remove-extra-lint branch from 84f4b51 to 7e33dd8 Compare January 15, 2025 10:32

Akhilesh-max force-pushed the remove-extra-lint branch from eef777e to 3772a69 Compare January 15, 2025 13:49

Akhilesh-max force-pushed the remove-extra-lint branch from 3772a69 to e1b6e57 Compare January 15, 2025 15:43

setrofim approved these changes Jan 15, 2025

View reviewed changes

yogeshbdeshpande approved these changes Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge "extra" lint target into "normal" lint target #153

Merge "extra" lint target into "normal" lint target #153

Akhilesh-max commented Jan 13, 2025 •

edited

Loading

Akhilesh-max commented Jan 13, 2025

Akhilesh-max commented Jan 14, 2025

setrofim commented Jan 14, 2025

yogeshbdeshpande commented Jan 14, 2025

yogeshbdeshpande Jan 14, 2025 •

edited

Loading

thomas-fossati Jan 14, 2025

yogeshbdeshpande Jan 14, 2025

thomas-fossati Jan 14, 2025

yogeshbdeshpande Jan 14, 2025 •

edited by thomas-fossati

Loading

thomas-fossati Jan 14, 2025

yogeshbdeshpande Jan 14, 2025 •

edited

Loading

yogeshbdeshpande left a comment

thomas-fossati Jan 14, 2025

setrofim Jan 14, 2025

thomas-fossati Jan 14, 2025

setrofim Jan 14, 2025

Akhilesh-max Jan 15, 2025 •

edited

Loading

setrofim Jan 15, 2025

Akhilesh-max Jan 15, 2025 •

edited

Loading

setrofim Jan 15, 2025

thomas-fossati Jan 14, 2025

Akhilesh-max Jan 15, 2025

thomas-fossati Jan 14, 2025

Akhilesh-max Jan 15, 2025

setrofim commented Jan 14, 2025

Akhilesh-max commented Jan 15, 2025

setrofim left a comment

Akhilesh-max commented Jan 15, 2025

yogeshbdeshpande left a comment

Merge "extra" lint target into "normal" lint target #153

Are you sure you want to change the base?

Merge "extra" lint target into "normal" lint target #153

Conversation

Akhilesh-max commented Jan 13, 2025 • edited Loading

Akhilesh-max commented Jan 13, 2025

Akhilesh-max commented Jan 14, 2025

setrofim commented Jan 14, 2025

yogeshbdeshpande commented Jan 14, 2025

yogeshbdeshpande Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yogeshbdeshpande Jan 14, 2025 • edited by thomas-fossati Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yogeshbdeshpande Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

yogeshbdeshpande left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akhilesh-max Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akhilesh-max Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

setrofim commented Jan 14, 2025

Akhilesh-max commented Jan 15, 2025

setrofim left a comment

Choose a reason for hiding this comment

Akhilesh-max commented Jan 15, 2025

yogeshbdeshpande left a comment

Choose a reason for hiding this comment

Akhilesh-max commented Jan 13, 2025 •

edited

Loading

yogeshbdeshpande Jan 14, 2025 •

edited

Loading

yogeshbdeshpande Jan 14, 2025 •

edited by thomas-fossati

Loading

yogeshbdeshpande Jan 14, 2025 •

edited

Loading

Akhilesh-max Jan 15, 2025 •

edited

Loading

Akhilesh-max Jan 15, 2025 •

edited

Loading